Skip to content

Dotnet SDK changes for 2019-10-01 Tags API for resources and subscriptions#9345

Merged
isra-fel merged 8 commits intoAzure:masterfrom
Grayer123:feature/tagApi-2019-10-01
Jan 29, 2020
Merged

Dotnet SDK changes for 2019-10-01 Tags API for resources and subscriptions#9345
isra-fel merged 8 commits intoAzure:masterfrom
Grayer123:feature/tagApi-2019-10-01

Conversation

@Grayer123
Copy link
Copy Markdown
Contributor

@Grayer123 Grayer123 commented Jan 7, 2020 •
edited
Loading

Update Generated code;
Add tag related test cases;
Update SessionRecords due to API version change.

Links to the swagger spec review PR:

TagApi-2019-10-01 changes: Update Generated code; add tag related tes…
88d5463 
…t cases; Update SessionRecords due to api version change.
@Grayer123 Grayer123 requested a review from erich-wang as a code owner January 7, 2020 02:51
@isra-fel isra-fel added Mgmt This issue is related to a management package. needs-review labels Jan 7, 2020
@isra-fel isra-fel self-assigned this Jan 7, 2020
@isra-fel isra-fel removed their assignment Jan 8, 2020
@Grayer123 Grayer123 requested a review from isra-fel as a code owner January 11, 2020 00:13
@isra-fel isra-fel self-assigned this Jan 13, 2020
@isra-fel
Copy link
Copy Markdown
Member

isra-fel commented Jan 13, 2020

Hi @Grayer123 please remember to remove the "[Draft]" or "WIP" or anything like that otherwise the review request might have been ignored...

@Grayer123 Grayer123 changed the title [Draft] Dotnet SDK changes for 2019-10-01 Tags API for resources and subscriptions Dotnet SDK changes for 2019-10-01 Tags API for resources and subscriptions Jan 13, 2020
isra-fel
isra-fel requested changes Jan 13, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Grayer123 , could you

  • Add a link to the swagger spec review PR in the description
  • Include the .txt file generated when you run generate.ps1(details)
  • Release notes in csproj
  • Update versions in csproj and assemblyinfo.cs if you are going to release

Thanks

@Grayer123
Copy link
Copy Markdown
Contributor Author

Grayer123 commented Jan 21, 2020

Hello @isra-fel, we have a question regarding autorest swagger to generate sdk code. When we specify an api parameter like:
{
"name": "api-version",
"in": "query",
"required": true,
"type": "string",
"description": "The API version to use for the operation."
}
in the "parameters": [] for an api operation in swagger,
it actually generate a parameter in SDK api operation like:
void CreateOrUpdate (... string api-version...);

But if we specify like this:
{
"$ref": "#/parameters/ApiVersionParameter"
}
(We define ApiVersionParameter with the same definition as above in the Parameters {} section at the bottom of swagger file)
in the swagger "parameters": [], in the SDK generated operation, it will generate like:
void CreateOrUpdate (...), without string api-version as a parameter.
Then we need to rely on the ResourceManagementClient.ApiVersion for this one.

We want to know what's the difference here for generating code with different defining methods, which related to a previous swagger change we made.

Many thanks.

@isra-fel
Copy link
Copy Markdown
Member

isra-fel commented Jan 22, 2020

Hi @Grayer123 I'm not sure about how C# generator handle them different. I suppose you can submit an issue in https://github.com/Azure/autorest.csharp

@Grayer123
Copy link
Copy Markdown
Contributor Author

Grayer123 commented Jan 23, 2020

Hello @isra-fel, thanks for the info, I have created a new issue within autorest.
Added links to the swagger spec review PR in the description; included the .txt file generated by generate.ps1; modified release notes and updated assembly version.

Also, for the above failed checks:
one is related to Microsoft.Azure.EventHubs.Tests, which I did not make any changes. (only modified Microsoft.Azure.Management.Resource);
another one is related to "Git checkout failed".

Could you please help us re-trigger new checks? Thanks.

@isra-fel
Copy link
Copy Markdown
Member

isra-fel commented Jan 24, 2020

/azp run net - mgmt - ci

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Jan 24, 2020

Azure Pipelines successfully started running 1 pipeline(s).

@Grayer123
Copy link
Copy Markdown
Contributor Author

Grayer123 commented Jan 24, 2020

Hello @isra-fel, the error went away when I re-merged from the master branch. Could you please kindly do a review of this pr?

isra-fel
isra-fel requested changes Jan 25, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Grayer123 , just one thing left: please update AzSdk.RP.props so that correct API version can be displayed on nuget.org page. Then I will merge the PR. THanks

@isra-fel isra-fel assigned Grayer123 and unassigned isra-fel Jan 25, 2020
@Grayer123
Copy link
Copy Markdown
Contributor Author

Grayer123 commented Jan 27, 2020

Hello @isra-fel, I have updated AzSdk.RP.props, please review one more time. Thanks

@isra-fel isra-fel merged commit 9bcab47 into Azure:master Jan 29, 2020
@Grayer123 Grayer123 mentioned this pull request Feb 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@isra-fel isra-fel isra-fel approved these changes

@erich-wang erich-wang Awaiting requested review from erich-wang

+1 more reviewer

@hamidtirmizi hamidtirmizi hamidtirmizi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Grayer123 Grayer123

Labels

Mgmt This issue is related to a management package.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Grayer123 @isra-fel @hamidtirmizi